unlink an ldap domain #11962
unlink an ldap domain #11962
Conversation
|
@blueorangutan package |
|
@DaanHoogland a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress. |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #11962 +/- ##
=========================================
Coverage 17.56% 17.56%
- Complexity 15534 15549 +15
=========================================
Files 5911 5914 +3
Lines 529359 529455 +96
Branches 64655 64679 +24
=========================================
+ Hits 92957 93002 +45
- Misses 425945 425991 +46
- Partials 10457 10462 +5
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull Request Overview
This PR adds functionality to unlink a CloudStack domain from LDAP, complementing the existing linkDomainToLdap functionality. The changes also include refactoring improvements to clean up the LinkDomainToLdapCmd by removing deprecated parameters and improving logging.
Key Changes
- Added new
unlinkDomainFromLdapAPI command to remove domain-to-LDAP linkages - Removed deprecated
nameparameter and improved theLinkDomainToLdapCmdimplementation - Applied minor code modernization (diamond operator, parametrized logging)
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| UnlinkDomainFromLdapCmd.java | New API command for unlinking domains from LDAP |
| LdapManager.java | Added interface method for unlinking and removed trailing semicolon from enum |
| LdapManagerImpl.java | Implemented unlinkDomainFromLdap method and applied diamond operator refactoring |
| LinkDomainToLdapCmd.java | Removed deprecated name parameter, made ldapDomain required, and improved logging |
| pom.xml | Added explicit cloud-api dependency |
Comments suppressed due to low confidence (1)
plugins/user-authenticators/ldap/src/main/java/org/apache/cloudstack/ldap/LdapManagerImpl.java:308
- The new UnlinkDomainFromLdapCmd class is not registered in the getCommands() list, which means it won't be available as an API command. Add
cmdList.add(UnlinkDomainFromLdapCmd.class);before the return statement.
final List<Class<?>> cmdList = new ArrayList<>();
cmdList.add(LdapUserSearchCmd.class);
cmdList.add(LdapListUsersCmd.class);
cmdList.add(LdapAddConfigurationCmd.class);
cmdList.add(LdapDeleteConfigurationCmd.class);
cmdList.add(LdapListConfigurationCmd.class);
cmdList.add(LdapCreateAccountCmd.class);
cmdList.add(LdapImportUsersCmd.class);
cmdList.add(LDAPConfigCmd.class);
cmdList.add(LDAPRemoveCmd.class);
cmdList.add(LinkDomainToLdapCmd.class);
cmdList.add(LinkAccountToLdapCmd.class);
return cmdList;
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
...enticators/ldap/src/main/java/org/apache/cloudstack/api/command/UnlinkDomainFromLdapCmd.java
Outdated
Show resolved
Hide resolved
shwstppr
left a comment
shwstppr
left a comment
There was a problem hiding this comment.
some comments, idea looks good. Will need testing
...enticators/ldap/src/main/java/org/apache/cloudstack/api/command/UnlinkDomainFromLdapCmd.java
Outdated
Show resolved
Hide resolved
...enticators/ldap/src/main/java/org/apache/cloudstack/api/command/UnlinkDomainFromLdapCmd.java
Outdated
Show resolved
Hide resolved
vishesh92
left a comment
vishesh92
left a comment
There was a problem hiding this comment.
Code looks good to me. Just one change is required.
| @Override | ||
| public List<Class<?>> getCommands() { | ||
| final List<Class<?>> cmdList = new ArrayList<Class<?>>(); | ||
| final List<Class<?>> cmdList = new ArrayList<>(); |
There was a problem hiding this comment.
Do we need to add the unlinkDomainFromLdap command here?
kiranchavala
left a comment
kiranchavala
left a comment
There was a problem hiding this comment.
(localcloud) 🐱 > link domaintoldap domainid=0735d7e8-5dcc-4b48-8049-81c69d8830d3 type=GROUP accounttype=2 ldapdomain=cn=qa-team,ou=Telco-Bng,dc=example,dc=in name=qa admin=admin
{
"LinkDomainToLdap": {
"accounttype": 2,
"domainid": "0735d7e8-5dcc-4b48-8049-81c69d8830d3",
"ldapdomain": "cn=qa-team,ou=Telco-Bng,dc=example,dc=in",
"name": "cn=qa-team,ou=Telco-Bng,dc=example,dc=in",
"type": "GROUP"
}
}
mysql> select * from ldap_configuration;
+----+-----------+------+-----------+--------------------------------------+
| id | hostname | port | domain_id | uuid |
+----+-----------+------+-----------+--------------------------------------+
| 2 | localhost | 389 | 2 | e07853d9-73dc-4486-9acf-66937c8123a5 |
+----+-----------+------+-----------+--------------------------------------+
1 row in set (0.00 sec)
mysql> select * from ldap_trust_map;
+----+-----------+-------+------------------------------------------+--------------+------------+
| id | domain_id | type | name | account_type | account_id |
+----+-----------+-------+------------------------------------------+--------------+------------+
| 1 | 2 | GROUP | cn=qa-team,ou=Telco-Bng,dc=example,dc=in | 2 | 0 |
+----+-----------+-------+------------------------------------------+--------------+------------+
1 row in set (0.00 sec)
Getting the following response from the api , but the entry is deleted from the database
(localcloud) 🐱 > unlink domainfromldap domainid=0735d7e8-5dcc-4b48-8049-81c69d8830d3
🙈 Error: failed to decode response
mysql> select * from ldap_trust_map;
Empty set (0.00 sec)
Also the UI progress doesn't stop when a user tried to link domain to ldap
|
@kiranchavala , those issues are fixed, however there are some polish issues remaining, like the condition to enable link or unlink are not available in the UI atm and I need to decide/discuss how to address these. |
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 15643 |
Co-authored-by: Abhishek Kumar <[email protected]>
Co-authored-by: Abhishek Kumar <[email protected]>
DaanHoogland
force-pushed
the
ghi3685-unlinkLdap
branch
from
e2bf6fd to
4c5e20c
Compare
|
@DaanHoogland, there is a similar use case with accounts where there are no UI options https://cloudstack.apache.org/api/apidocs-4.20/apis/linkAccountToLdap.html. Should this also be considered in this PR? |
The improvement issue is already present |
guys (@rajujith @kiranchavala), I don’t want to add and create a big beautiful PR. I’d rather implement smaller well tested changes, if you don’t mind. We need to have a backend change in |
Pearl1594
left a comment
Pearl1594
left a comment
There was a problem hiding this comment.
Linking and unlinking of domain to ldap works fine, however, as an improvement to this it would be nice to selectively show and hide the link/unlink buttons based on whether the domain is linked to an LDAP or not. But that could happen on a separate PR> Also, ldapdomain requires translation
7 participants
Description
This PR
Fixes: #3685 partially as unlinking an account has no good functional definition (yet)
Fixes: #11474 by removing a long time deprecated parameter
Types of changes
Feature/Enhancement Scale or Bug Severity
Feature/Enhancement Scale
Bug Severity
Screenshots (if appropriate):
How Has This Been Tested?
How did you try to break this feature and the system with this change?